feat: handle nam as denominated amount #1301
Conversation
mateuszjasiuk
requested review from
jurevans,
euharrison,
pedrorezende and
emccorson
and removed request for
jurevans,
euharrison,
pedrorezende and
emccorson
mateuszjasiuk
force-pushed
the
feat/handle-queried-nam-as-denominated-amount
branch
from
c4220b5
to
97750a1
Compare
mateuszjasiuk
force-pushed
the
feat/handle-queried-nam-as-denominated-amount
branch
from
97750a1
to
0f1d83d
Compare
mateuszjasiuk
force-pushed
the
feat/handle-queried-nam-as-denominated-amount
branch
from
0f1d83d
to
afbcacc
Compare
mateuszjasiuk
requested review from
pedrorezende,
euharrison,
emccorson and
jurevans
and removed request for
pedrorezende,
euharrison and
emccorson
mateuszjasiuk
force-pushed
the
feat/handle-queried-nam-as-denominated-amount
branch
from
5bc89d8
to
ebc47b3
Compare
mateuszjasiuk
force-pushed
the
feat/handle-queried-nam-as-denominated-amount
branch
from
f333ac5
to
dd3a3ae
Compare
|
|
||
| // We can't return "satisfies Asset" from fn | ||
| // eslint-disable-next-line @typescript-eslint/explicit-function-return-type | ||
| export const namadaAsset = () => { |
There was a problem hiding this comment.
I'm removing this implementation and moving it to namada-chain-registry #1308
Do you think it could have some impact here?
There was a problem hiding this comment.
Should be fine I can fix conflict later
There was a problem hiding this comment.
Sorry, the conflict arrived 🙈 #1308
apps/namadillo/jest.config.ts
Outdated
| "ts-jest", | ||
| { | ||
| diagnostics: { | ||
| ignoreCodes: [1343], |
There was a problem hiding this comment.
could you add some reference why we need this?
There was a problem hiding this comment.
I can try xD
There was a problem hiding this comment.
tbh with current solution this might not be needed. Just jest is having problems resolving import.meta.env syntax. I will remove this all togehter if it works fine witout it
| ...internalDevnetAssets, | ||
| chain_name: "localnet", | ||
| assets: internalDevnetAssets.assets.map((asset) => | ||
| asset.name === "NAM" ? |
There was a problem hiding this comment.
I think this is not going to work once we're using the registry with anoma/namada-chain-registry#13 merged. Maybe could use "Namada" or check against symbol or base instead?
|
|
||
| export const toDisplayAmount = ( | ||
| asset: Asset, | ||
| baseAmount: BigNumber | ||
| ): BigNumber => { | ||
| if (isNamAsset(asset)) return baseAmount; |
There was a problem hiding this comment.
I think this won't work for NAM transferred from other chains. The indexer will return the balances in display denom for NAM from the current chain, but in base denom for NAM from other chains. But this check here will assume any token with symbol as NAM is in display denom.
I was wondering if this check should be outside the toDisplayAmount function anyway, since to me it seems like that logic is outside of the scope of the function. What do you think?
There was a problem hiding this comment.
Actually, I just also realised I think it's not going to work for displaying NAM on Cosmos chains either. The NAM balance we get from a Cosmos chain will be in base denom, but this check will stop it being converted to display denom.
There was a problem hiding this comment.
yeah maybe moving this outside makes sense :D Just so I understand better, what do you mean by "NAM transferred from other chains." For NAM to arrive on other chain it first has to be sent from namada chain. So if we send NAM to cosmos and we send it back, will be it treated as different token than regular NAM?
There was a problem hiding this comment.
The two cases I think won't work are:
- NAM that has been sent to a Cosmos chain (since the balance query for cosmjs returns amounts in base denom)
- NAM from something other than the current chain e.g. your current chain is housefire but you sent some NAM from internal devnet to housefire
In both cases, these should display properly as the Namada asset in Namadillo, but the denomation will be wrong in this PR because Namadillo thinks it's already in display denom but it's actually in base denom.
The case you're talking about where you send NAM from Namada to Cosmos and then back should work fine I think, because the indexer will still return the balance using the original tnam address and in display denom (as far as I know).
(Also, even though in case 2, the NAM from internal devnet displays as the Namada asset, it is a different thing from NAM from housefire, but we don't actually indicate that anywhere right now.)
Indexer part: anoma/namada-indexer#157
Issue: anoma/namada-indexer#156
Changes:
Added envs so you can optionally develop against localnet. You have to specify:VITE_LOCALNET=true, VITE_LOCALNET_CHAIN_ID, VITE_LOCALNET_NAM_TOKENlocalnet-config.toml, by default added to gitignore. You can add it tonamadillo/public/localnet-config.tomlif you want to develop against local chain. Example file: